Skip to content

http2: Moves HTTP/2 GOAWAY load shed point handling to the ConnectionManager#44736

Open
birenroy wants to merge 8 commits into
envoyproxy:mainfrom
birenroy:fix-http2-loadshedding
Open

http2: Moves HTTP/2 GOAWAY load shed point handling to the ConnectionManager#44736
birenroy wants to merge 8 commits into
envoyproxy:mainfrom
birenroy:fix-http2-loadshedding

Conversation

@birenroy
Copy link
Copy Markdown
Contributor

@birenroy birenroy commented Apr 29, 2026 •
edited
Loading

As implemented, the http2_server_go_away_on_dispatch load shed point causes Envoy to send a single GOAWAY frame, with the max_stream_id set to the highest stream ID observed by the server at that point. This results in the server dropping any requests in flight at that moment, making the load shed point unusable in practice.

This PR moves the load shed point to the connection manager, where it can initiate the graceful shutdown sequence described in RFC 9113 Section 6.8, by calling ConnectionManagerImpl::sendGoAwayAndClose().

For symmetry, I have also moved the http2_server_go_away_and_close_on_dispatch load shed point, maintaining the eager shutdown behavior.

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@birenroy
Moves HTTP/2 GOAWAY load shed point handling to the ConnectionManager.
cba00d7 
Signed-off-by: Biren Roy <birenroy@google.com>
@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only Bot commented Apr 29, 2026

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #44736 was opened by birenroy.

see: more, trace.

@birenroy
Copy link
Copy Markdown
Contributor Author

birenroy commented May 5, 2026

/retest

@birenroy
Added some test cases.
945d950 
Signed-off-by: Biren Roy <birenroy@google.com>
@birenroy birenroy marked this pull request as ready for review May 6, 2026 03:34
@birenroy
Copy link
Copy Markdown
Contributor Author

birenroy commented May 6, 2026

/assign @RyanTheOptimist
/cc @AdiJohn @pradeepcrao

@birenroy
Reduces test flakiness.
2cd26a9 
Signed-off-by: Biren Roy <birenroy@google.com>
@birenroy
Copy link
Copy Markdown
Contributor Author

birenroy commented May 6, 2026

/retest

@birenroy
One last fix for test flakiness.
c13545e 
Signed-off-by: Biren Roy <birenroy@google.com>
@birenroy
Copy link
Copy Markdown
Contributor Author

birenroy commented May 7, 2026

/assign @paul-r-gall

@birenroy
Merged upstream
c7cb710 
Signed-off-by: Biren Roy <birenroy@google.com>
@birenroy
Copy link
Copy Markdown
Contributor Author

birenroy commented May 8, 2026

/retest

RyanTheOptimist
RyanTheOptimist previously approved these changes May 11, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change LGTM, but I do wonder if it needs a runtime guard as this is a behavior change. I agree with your assessment that it's a bad idea to use the load shed point as-is, but someone might be doing so. WDYT?

@RyanTheOptimist
Copy link
Copy Markdown
Contributor

RyanTheOptimist commented May 14, 2026

/wait

@birenroy
Added runtime feature.
a07f2f2 
Signed-off-by: Biren Roy <birenroy@google.com>
@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only Bot commented May 14, 2026

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #44736 was synchronize by birenroy.

see: more, trace.

@birenroy
Copy link
Copy Markdown
Contributor Author

birenroy commented May 14, 2026

This change LGTM, but I do wonder if it needs a runtime guard as this is a behavior change. I agree with your assessment that it's a bad idea to use the load shed point as-is, but someone might be doing so. WDYT?

I added a runtime feature. Thanks!

@birenroy
Added a changelog entry.
677b32f 
Signed-off-by: Biren Roy <birenroy@google.com>
RyanTheOptimist
RyanTheOptimist previously approved these changes May 14, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the "just to be safe" runtime guard!

@RyanTheOptimist RyanTheOptimist enabled auto-merge (squash) May 14, 2026 23:06
@birenroy
Trying to fix coverage.
88aa8e3 
Signed-off-by: Biren Roy <birenroy@google.com>
auto-merge was automatically disabled May 15, 2026 13:26

Head branch was pushed to by a user without write access

@birenroy
Copy link
Copy Markdown
Contributor Author

birenroy commented May 15, 2026

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RyanTheOptimist RyanTheOptimist RyanTheOptimist left review comments

@yanavlasov yanavlasov Awaiting requested review from yanavlasov

@mattklein123 mattklein123 Awaiting requested review from mattklein123

At least 1 approving review is required to merge this pull request.

Assignees

@paul-r-gall paul-r-gall

@RyanTheOptimist RyanTheOptimist

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@birenroy @RyanTheOptimist @paul-r-gall